Node SDK: Handle tool and permission broadcasts via event model#686
Node SDK: Handle tool and permission broadcasts via event model#686SteveSandersonMS wants to merge 11 commits intomainfrom
Conversation
…callbacks Update the Node SDK to listen for broadcast events (external_tool.requested, permission.requested) and respond via sharedApi RPC methods (session.tools.handlePendingToolCall, session.permissions.handlePendingPermissionRequest) instead of the old tool.call/permission.request RPC handlers. - Regenerate rpc.ts and session-events.ts from runtime schemas - Add _handleBroadcastEvent to session.ts for event interception - Remove old RPC handlers from client.ts - Add useStdio option to test harness for TCP mode Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Verify that when two SDK clients are connected to the same CLI process, both see external_tool.requested/completed and permission.requested/completed events. The permission test has client 1 manually approve while client 2 observes, confirming the broadcast model works across connections. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ent 1 approves Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…rtions Add E2E test verifying one client can reject a permission and both clients see the denial in the permission.completed event. Regenerate SDK types from updated runtime schemas so permission.completed includes result.kind enum. Use type-narrowing filters instead of casts in test assertions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…leanup - two clients register different tools and agent uses both: client1 has city_lookup, client2 has currency_lookup, verifies union semantics - disconnecting client removes its tools: client2's ephemeral_tool removed after disconnect, client1's stable_tool persists - Update existing test: client2 resumes with NO tools (doesn't overwrite) - Regenerated snapshots for existing tests (minor wording changes) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Bump SDK_PROTOCOL_VERSION from 2 to 3 to match runtime breaking change - Wrap fallback RPC calls in _executeToolAndRespond and _executePermissionAndRespond with try/catch to prevent unhandled rejections if the connection is disposed mid-flight Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
These are no different from any other event handler — let rejections propagate naturally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Cross-SDK Consistency ReviewThis PR introduces a breaking architectural change to the Node.js SDK by migrating from an RPC callback model to a broadcast event model for handling tool calls and permission requests. This change bumps the protocol version from 2 to 3. 🚨 Critical Inconsistency: Protocol Version MismatchNode.js SDK (this PR):
Python, Go, and .NET SDKs (current state):
ImpactThis creates a breaking inconsistency across the SDK implementations:
RecommendationTo maintain cross-SDK consistency, all four SDKs should be updated together to implement the broadcast event model and bump to protocol version 3: Required changes for Python SDK:
Required changes for Go SDK:
Required changes for .NET SDK:
Alternative ApproachIf updating all SDKs simultaneously is not feasible, consider:
NoteThe PR description mentions: "
|
There was a problem hiding this comment.
Pull request overview
This PR updates the Node SDK to handle external tool calls and permission requests via broadcast session events (instead of per-connection RPC callbacks), aligns the Node client with a breaking runtime protocol change, and adds multi-client E2E coverage for the new behavior.
Changes:
- Switch tool/permission handling to the broadcast event model by intercepting
external_tool.requestedandpermission.requestedinCopilotSessionand responding via new session RPC methods. - Bump Node
SDK_PROTOCOL_VERSIONto 3 and regenerate protocol typings (rpc.ts,session-events.ts) to match updated runtime schemas. - Update/add E2E tests and snapshots, including new multi-client broadcast scenarios.
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| nodejs/src/client.ts | Removes legacy tool.call / permission.request request handlers and relies on session event dispatch for tool/permission handling. |
| nodejs/src/session.ts | Adds internal handling for broadcast request events and responds via new session RPC methods. |
| nodejs/src/sdkProtocolVersion.ts | Bumps Node SDK protocol version from 2 → 3. |
| nodejs/src/generated/rpc.ts | Adds new session RPC methods for responding to pending tool calls and permission requests; updates some schema fields. |
| nodejs/src/generated/session-events.ts | Regenerates/expands session event types, including new external tool events and updated permission completion payload. |
| nodejs/test/e2e/harness/sdkTestContext.ts | Adds useStdio option to allow TCP-mode tests needed for multi-client scenarios. |
| nodejs/test/e2e/multi-client.test.ts | Adds new multi-client E2E tests validating broadcast semantics for tools and permissions. |
| test/snapshots/tools/*.yaml | Updates tool-related snapshots to reflect new runtime/SDK behavior and outputs. |
| test/snapshots/multi_client/*.yaml | Adds new snapshots for multi-client broadcast scenarios. |
Comments suppressed due to low confidence (1)
nodejs/src/client.ts:1281
tool.callrequest handling (and the privatehandleToolCallRequesthelper) was removed here, but there is still a unit test referencinghandleToolCallRequest(seenodejs/test/client.test.ts). This will fail TypeScript compilation/tests until that test is updated or removed to reflect the new broadcast-event tool handling model.
this.connection.onNotification("session.event", (notification: unknown) => {
this.handleSessionEventNotification(notification);
});
this.connection.onNotification("session.lifecycle", (notification: unknown) => {
this.handleSessionLifecycleNotification(notification);
});
// External tool calls and permission requests are now handled via broadcast events:
// the server sends external_tool.requested / permission.requested as session event
// notifications, and CopilotSession._dispatchEvent handles them internally by
// executing the handler and responding via session.tools.respond / session.permissions.respond RPC.
- Remove unused session2 variable in multi-client test - Document fire-and-forget semantics in _handleBroadcastEvent - Fix null/undefined handling in tool result serialization - Fix stale RPC method names in comment Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
| SDK | Protocol Version | Tool/Permission Pattern | Multi-Client Support |
|---|---|---|---|
| Node.js | 3 ✅ | Broadcast events ✅ | Yes ✅ |
| Python | 2 ❌ | RPC callbacks ❌ | No ❌ |
| Go | 2 ❌ | RPC callbacks ❌ | No ❌ |
| .NET | 2 ❌ | RPC callbacks ❌ | No ❌ |
Key Inconsistencies
1. Protocol Version Mismatch
The Node SDK bumps to protocol version 3, while Python, Go, and .NET remain on version 2. This means:
- Node SDK clients will only work with runtime version 3+
- Other SDKs will only work with runtime version 2
- Users cannot mix SDK languages in multi-client scenarios
Files affected:
- ✅
nodejs/src/sdkProtocolVersion.ts- bumped to 3 - ❌
python/copilot/sdk_protocol_version.py- still 2 - ❌
go/sdk_protocol_version.go- still 2 - ❌
dotnet/src/SdkProtocolVersion.cs- still 2
2. Tool & Permission Handling Pattern
Node SDK now uses broadcast events (external_tool.requested, permission.requested) and responds via new RPC methods, while other SDKs still use the old callback pattern:
Node.js (new pattern):
- Listen for
external_tool.requested/permission.requestedsession events - Respond via
session.tools.handlePendingToolCall/session.permissions.handlePendingPermissionRequestRPC
Python/Go/.NET (old pattern):
- Still register
tool.call/permission.requestRPC request handlers - Respond directly to the RPC request
Files that need updates:
- ❌
python/copilot/client.py:1351-1352- still setstool.callandpermission.requesthandlers - ❌
go/client.go:1289-1290- still setstool.callandpermission.requesthandlers - ❌
dotnet/src/Client.cs:1125-1126- still setstool.callandpermission.requesthandlers
3. Multi-Client Support
The Node SDK now supports multiple clients connecting to the same session (all receive broadcast events). This capability doesn't exist in other SDKs and would require:
- Event handling for broadcast request events
- Coordination of responses from multiple clients
- E2E tests for multi-client scenarios
Files that demonstrate this:
- ✅
nodejs/test/e2e/multi-client.test.ts- 5 new multi-client test cases - ❌ Python, Go, .NET - no multi-client tests or support
4. New Extension Export
Node SDK adds extension.ts export for child process scenarios - unclear if this is needed in other SDKs.
Recommendations
To maintain cross-SDK consistency, the following options should be considered:
Option A: Update all SDKs in parallel
- Port the broadcast event model to Python, Go, and .NET
- Bump all SDKs to protocol version 3 simultaneously
- Add multi-client support and tests across all languages
- Ensures feature parity but requires significant coordination
Option B: Staged rollout with version compatibility
- Document that Node SDK v3 requires runtime v3+
- Plan follow-up PRs to port changes to other SDKs
- Track the migration in a GitHub issue
- Temporary inconsistency is acceptable if tracked and planned
Option C: Keep protocol version 2 support in Node SDK
- Make the Node SDK backward compatible with both v2 and v3 protocols
- Allow gradual migration of other SDKs
- More complex but maintains broader compatibility
Questions for Maintainers
- Is there a timeline for porting these changes to Python, Go, and .NET?
- Should protocol version 3 support be tracked as a feature gap in other SDKs?
- Are there any runtime compatibility considerations that affect the rollout strategy?
- Should this PR be labeled as breaking change / requires coordination across SDKs?
Note: This is a technical consistency review, not a judgment on code quality. The Node SDK implementation looks well-designed - the concern is ensuring the other SDKs evolve in parallel to maintain API parity across languages.
Generated by SDK Consistency Review Agent for issue #686 · ◷
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #686
| * This must match the version expected by the copilot-agent-runtime server. | ||
| */ | ||
| export const SDK_PROTOCOL_VERSION = 2; | ||
| export const SDK_PROTOCOL_VERSION = 3; |
There was a problem hiding this comment.
Cross-SDK Consistency: This protocol version bump to 3 is not reflected in the other SDKs:
- Python:
python/copilot/sdk_protocol_version.py- still at version 2 - Go:
go/sdk_protocol_version.go- still at version 2 - .NET:
dotnet/src/SdkProtocolVersion.cs- still at version 2
This creates a compatibility issue where Node SDK clients will only work with runtime v3+, while other SDK clients remain on v2. Consider coordinating this version bump across all SDKs or documenting the migration plan.
| @@ -0,0 +1,289 @@ | |||
| /*--------------------------------------------------------------------------------------------- | |||
There was a problem hiding this comment.
Cross-SDK Consistency: These multi-client E2E tests demonstrate new functionality that doesn't exist in the other SDKs. For feature parity, similar tests should be added to:
python/e2e/- no multi-client tests currentlygo/- no multi-client tests currentlydotnet/test/- no multi-client tests currently
The multi-client capability requires the broadcast event model, so these tests can't be ported until the other SDKs implement the same pattern.
| * rejections, consistent with standard EventEmitter / event handler semantics. | ||
| * @internal | ||
| */ | ||
| private _handleBroadcastEvent(event: SessionEvent): void { |
There was a problem hiding this comment.
Cross-SDK Consistency: This new broadcast event handling pattern for external_tool.requested and permission.requested is Node-only. The other SDKs still use the old RPC callback pattern:
- Python:
client.py:1351-1352- still registerstool.callandpermission.requestRPC handlers - Go:
client.go:1289-1290- same old pattern - .NET:
Client.cs:1125-1126- same old pattern
For consistency, these SDKs would need similar _handleBroadcastEvent implementations that:
- Listen for broadcast session events instead of RPC requests
- Execute handlers locally
- Respond via the new RPC methods (
session.tools.handlePendingToolCall,session.permissions.handlePendingPermissionRequest)
|
|
||
| import { CopilotClient } from "./client.js"; | ||
|
|
||
| export const extension = new CopilotClient({ isChildProcess: true }); |
There was a problem hiding this comment.
Cross-SDK Consistency: This new extension export for child process scenarios is Node-only.
Questions:
- Is this functionality needed in other SDKs?
- Should Python, Go, and .NET provide equivalent child process integration patterns?
- Is this a Node-specific use case or a general SDK feature?
If this is generally useful, consider adding equivalent exports in the other SDKs.
Cross-SDK Consistency ReviewThis PR introduces a significant architectural change to the Node.js SDK, replacing the RPC callback pattern for tool calls and permission requests with a broadcast event model. This is a breaking change (protocol version bumped from 2 → 3) that enables multi-client scenarios where multiple SDK clients can connect to the same CLI session. Summary of Changes (Node.js SDK)Before (Protocol v2): The runtime invoked After (Protocol v3): The runtime broadcasts
This enables:
|
Overview
Replace the RPC callback pattern for external tool calls and permission requests with a broadcast event model. The runtime now broadcasts
external_tool.requestedandpermission.requestedas session events to all connected clients, and clients respond viasession.tools.handlePendingToolCallandsession.permissions.handlePendingPermissionRequestRPC methods.Changes
Event-based handling (Node SDK):
tool.callandpermission.requestRPC request handlers fromclient.ts_handleBroadcastEventinsession.tsthat intercepts broadcast request events, executes local handlers, and responds via the new RPC methodsnormalizeToolResult,buildUnsupportedToolResult, and related helper methods that are no longer neededCodegen updates:
rpc.tsandsession-events.tsfrom updated runtime schemaspermission.completedevent now includesresult.kindenum for observing outcomesProtocol version:
SDK_PROTOCOL_VERSIONfrom 2 to 3 to match the runtime breaking changeE2E tests (5 new multi-client tests):
The runtime must include the broadcast model changes and per-connection tool tracking for these tests to pass. Until then, E2E tests that depend on the new protocol (multi-client tests, and any tests using protocol version 3) will fail.